Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Show options with Collapsible #7716

Closed
wants to merge 21 commits into from

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Jun 16, 2020

User facing changelog

Show command options with react-inspector.

Additional details

Why was this change necessary?

Sometimes, user-defined command options are unclear like Object{N} or even ignored on reporter.

What is affected by this change?

It shows the values of options on reporter.

Any implementation details to explain?

N/A

How has the user experience changed?

Before:

Screenshot from 2020-06-22 12-45-40

After:

Peek 2020-06-22 12-46

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • [N/A] Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • [N/A] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 16, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh force-pushed the issue-485-678 branch 4 times, most recently from 18b4685 to 92f9c56 Compare June 22, 2020 02:50
@sainthkh sainthkh marked this pull request as ready for review June 22, 2020 03:49
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the tests in https://github.com/cypress-io/cypress-demo-ui/blob/master/cypress/integration/ui-states.js#L240 (Basically the same tests that are in the log_options_spec.js file)

Visual changes

I like the options being more clearly defined, in this case with the use of an arrow icon.

Before

Screen Shot 2020-06-22 at 1 16 04 PM

Screen Shot 2020-06-22 at 1 16 49 PM

Screen Shot 2020-06-22 at 1 17 09 PM

After

Screen Shot 2020-06-22 at 1 30 22 PM

Screen Shot 2020-06-22 at 1 30 53 PM

Screen Shot 2020-06-22 at 1 31 07 PM

Screen Shot 2020-06-22 at 1 31 20 PM

Performance changes

This seems to be slowing down the rendering/painting of the commands when there are options passed, so that a test will run longer after this change.

I'm not sure if there's something we could handle better here.

Before

Screen Shot 2020-06-22 at 1 55 39 PM

After

Screen Shot 2020-06-22 at 1 55 55 PM

@sainthkh
Copy link
Contributor Author

uncaught errors logs error on page load when new page has uncaught exception - logs error on page load when new page has uncaught exception passes on local Ubuntu 20.04. But it failed again here.

packages/driver/src/cypress/log.js Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/actions/click.js Outdated Show resolved Hide resolved
@sainthkh
Copy link
Contributor Author

Flaky failure.

panzarino
panzarino previously approved these changes Jun 24, 2020
Copy link
Contributor

@panzarino panzarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me assuming performance issues pointed out by Jennifer are fixed / improved.

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sainthkh The visual changes look good and we're ok to move forward with them. Have a couple questions left.

  • Has there been any changes made to address the performance concerns?
  • How does this perform during cypress run. Since this isn't interact-able, so couldn't be expanded - is there any difference in the behavior? There probably should be some thought for this.

@sainthkh
Copy link
Contributor Author

sainthkh commented Jun 25, 2020

  • With cypress run, options tree is expanded by default.
  • Nothing has been done for performance yet. But the solution is a bit hard. I thought of lazy-calling react-inspector components. It might solve the problem of cypress open, but it cannot with cypress run because it's expanded by default. Maybe we need to give up react-inspector and create the simplified version of it.

@chrisbreiding
Copy link
Contributor

Nothing has been done for performance yet. But the solution is a bit hard. I thought of lazy-calling react-inspector components. It might solve the problem of cypress open, but it cannot with cypress run because it's expanded by default. Maybe we need to give up react-inspector and create the simplified version of it.

I agree we might be best off doing this without react-inspector. I don't think having nested collapsing is that useful for this case, there aren't even that many commands with options that require it anyway, and it's probably adding a lot of performance overhead. What do you think about using our own <Collapsible> component instead and have the options displayed as a string in a <pre>?

@sainthkh

This comment has been minimized.

@jennifer-shehane
Copy link
Member

@chrisbreiding The expand is mostly helpful for things that could have larger options (basically request, screenshot, setCookie, route, etc)

@sainthkh I think I would prefer it kept looking like an object, to more closely reflect the API. Maybe an inline object if one option:

get     input
.type   Acme Company
        { force: true }

And expanded object if more than one option? Or maybe react-inspector could be used only when more than one option? So you can expand only then.

get     input
.type   Acme Company
        { 
          force: true,
          delay: 10
        }

@chrisbreiding
Copy link
Contributor

The expand is mostly helpful for things that could have larger options (basically request, screenshot, setCookie, route, etc)

Sorry I wasn't clear. I was referring to options objects that have further objects or arrays nested inside of them. I agree that the root options object should be collapsed/expanded, but I don't see much benefit in collapsing nested objects. For example, if you did cy.screenshot({ log: true, blackout: ['.foo']), it might appear like this:

.screenshot 
  ▶️ { log: true }

Like this after you've expanded it:

.screenshot 
  🔽 { 
    log: true
    blackout: ▶️ [] 
  }

And then this after you clicked the blackout arrow:

.screenshot 
  🔽 {
    log: true
    blackout: 🔽 [
      '.foo'
	] 
  }

But I think it's fine to jump straight to the fully expanded version. This obviates the need for react-inspector and should make rendering more performant.

@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 6, 2020

I fixed the design.

Single line

A: With <pre>
Screenshot from 2020-07-06 15-22-16

B: No <pre>
Screenshot from 2020-07-06 15-45-12

I think B looks better than A. Or are there any other suggestions?

Collapsible

Peek 2020-07-06 15-36

Show collapsible when an object has 2+ properties or the type of its only property is not an object/array.

Future Plan

If the feature and design are right, I'll:

  • expand the collapsible by default in run mode.
  • write and fix tests.

@chrisbreiding
Copy link
Contributor

Definitely want to go with B. My suggestion for using pre was so that the dom would be simple but have the whitespace formatting of a pre tag. I didn't mean to suggest that we should use the pre CSS styling we have in place. Using CSS white-space: pre would work also and wouldn't inherit the pre CSS styling.

@sainthkh
Copy link
Contributor Author

Rebased and resolved conflicts.

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Dec 22, 2020

Part of why this was postponed for review is we were waiting to write some performance tests for the reporter - which has been done and exist here: https://github.com/cypress-io/cypress/blob/issue-6783/packages/driver/cypress/integration/e2e/performance_spec.ts

So, assuming that these tests test what we want and that these changes don't impact the performance - we should be able to pick this back up when we have some time.

@sainthkh
Copy link
Contributor Author

I fixed this because it's left unresolved for a long time. The #8805 is WIP and the code will change as the things go on. I'll come back when it's done.

@jennifer-shehane
Copy link
Member

@sainthkh Yah, we're taking a break from #8805 because the current implementation did not appear to resolve the original performance issues - based on the performance test metrics.

@badeball
Copy link
Contributor

badeball commented Feb 17, 2021

Yah, we're taking a break from #8805 because the current implementation did not appear to resolve the original performance issues - based on the performance test metrics.

That's very sad to hear. I tried the pre-release builds and I too did not experience any performance improvement in CI. Are you actively working on performance improvement at the moment? I see several issues regarding this has been closed recently, ref. #2579 and #4638.

I'm currently working on a test suite consisting of 80 specs, each which takes ~6 seconds to load. This in itself adds 8 minutes in accumulated execution time. Furthermore, every test logs around 100-200 commands, which seems to further suppress performance. At least it decimates observed / experienced performance in interactive mode.

I'm worried that Cypress doesn't take performance into account in the way I hoped and that it has become a tool which I will no longer consider.

@jennifer-shehane
Copy link
Member

@badeball Unfortunately we're not looking into the performance issues atm.

@sainthkh
Copy link
Contributor Author

sainthkh commented Mar 7, 2022

Closing because things have changed a lot.

@sainthkh sainthkh closed this Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants